Skip to content

Fix check for python version on sc2json script#179

Closed
manuelseeger wants to merge 4 commits intoggtracker:upstreamfrom
manuelseeger:upstream
Closed

Fix check for python version on sc2json script#179
manuelseeger wants to merge 4 commits intoggtracker:upstreamfrom
manuelseeger:upstream

Conversation

@manuelseeger
Copy link
Contributor

@manuelseeger manuelseeger commented Jan 8, 2023

Python3 deprecates encoding option on json.dumps and defaults to UTF8.

@manuelseeger
Remove encoding option from JSON script.
09a00cc 
Python3 deprecates encoding option on json.dumps and defaults to UTF8.
@manuelseeger
Copy link
Contributor Author

manuelseeger commented Jan 8, 2023

This was nonsense.

Scripts still errors for me though even with the TypeError. Is the executable delivered with the pip package out of date?

@manuelseeger
Remove encoding option from JSON script.
99e1071 
Python3 deprecates encoding option on json.dumps and defaults to UTF8.

The existing try didn't work since the plugin wrapper
doesn't call json.dumps until after except statement in line 41.
@manuelseeger
Also mask cmd line option to not show for python 3
d5f6c5d
@manuelseeger manuelseeger reopened this Jan 8, 2023
@cclauss
Copy link
Collaborator

cclauss commented Jan 8, 2023

Can you please provide the stack trace for the TypeError?

@manuelseeger
Copy link
Contributor Author

manuelseeger commented Jan 8, 2023

The existing TypeError check doesn't seem to work since the plugin wrapper doesn't invoke json dumps until after the replay is being loaded in line 41

@manuelseeger manuelseeger changed the title Remove encoding option from JSON script. Fix check for python version on sc2json script Jan 8, 2023
@manuelseeger
Copy link
Contributor Author

manuelseeger commented Jan 8, 2023

The TypeError is thrown on factory.load_replay, not on factory.register_plugin, which is being checked.

sc2reader/sc2reader/scripts/sc2json.py

Line 41 in 7da58b1

replay_json = factory.load_replay(args.path[0])

@cclauss
Copy link
Collaborator

cclauss commented Jan 8, 2023

The TypeError is thrown on factory.load_replay

Can you please provide the stack trace?

@cclauss
Copy link
Collaborator

cclauss commented Jan 8, 2023

#163 (comment)

@manuelseeger
Copy link
Contributor Author

manuelseeger commented Jan 8, 2023

Traceback (most recent call last):
File "C:\Users\manuel\dev\sc2reader\sc2reader\scripts\sc2json.py", line 46, in
main()
File "C:\Users\manuel\dev\sc2reader\sc2reader\scripts\sc2json.py", line 41, in main
replay_json = factory.load_replay(args.path[0])
File "c:\users\manuel\dev\sc2reader\sc2reader\factories\sc2factory.py", line 85, in load_replay
return self.load(Replay, source, options, **new_options)
File "c:\users\manuel\dev\sc2reader\sc2reader\factories\sc2factory.py", line 163, in load
return self._load(cls, resource, filename=filename, options=options)
File "c:\users\manuel\dev\sc2reader\sc2reader\factories\sc2factory.py", line 174, in load
obj = plugin(obj)
File "c:\users\manuel\dev\sc2reader\sc2reader\factories\plugins\utils.py", line 17, in call
return func(*args, **opt)
File "c:\users\manuel\dev\sc2reader\sc2reader\factories\plugins\replay.py", line 19, in toJSON
return json.dumps(obj, **options)
File "C:\Users\manuel.conda\envs\sc2reader\lib\json_init.py", line 234, in dumps
return cls(
TypeError: JSONEncoder.init() got an unexpected keyword argument 'encoding'

cclauss
cclauss reviewed Jan 8, 2023
View reviewed changes
sc2reader/scripts/sc2json.py
args = parser.parse_args()

factory = sc2reader.factories.SC2Factory()
try:
Copy link
Collaborator

@cclauss cclauss Jan 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change above is good.
The change below is not really required because the try / except block does the same thing.

Copy link
Contributor Author

@manuelseeger manuelseeger Jan 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I would leave the original try/except it would throw an AttributeError before the TypeError though if args.encoding doesn't exist

@StoicLoofah
Copy link
Collaborator

StoicLoofah commented Jan 9, 2023

If python3 doesn't support encoding, should we just drop the encoding option entirely from this command?

@cclauss
Copy link
Collaborator

cclauss commented Jan 9, 2023

should we just drop the encoding option entirely from this command?

Yes, but we should also drop Python 2… #163 (comment)

@StoicLoofah
Copy link
Collaborator

StoicLoofah commented Jan 9, 2023

should we just drop the encoding option entirely from this command?

Yes, but we should also drop Python 2… #163 (comment)

You already did a great job clearing out a bunch of Python 2 stuff in this PR #174 Is there more we need to do in the code to drop support, or is it just a config thing?

@cclauss
Copy link
Collaborator

cclauss commented Aug 30, 2023

@manuelseeger Can you please rebase this pull request or close it?

cclauss
cclauss reviewed Aug 30, 2023
View reviewed changes
sc2reader/scripts/sc2json.py
nargs=1,
help="Path to the replay to serialize.",
)
if sys.version_info.major < 3:
Copy link
Collaborator

@cclauss cclauss Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of #174 Python 2 is no longer supported.

@manuelseeger manuelseeger closed this by deleting the head repository Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@cclauss cclauss cclauss approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@manuelseeger @cclauss @StoicLoofah